-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add cert-manager plugin #143
base: main
Are you sure you want to change the base?
Conversation
edef4f5
to
b8ebea9
Compare
This is great 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a couple of small comments. The plugin should also display a special error if the cert-manager is not installed.
}, | ||
{ | ||
label: 'Last Transition Time', | ||
getter: item => item.lastTransitionTime, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Signed-off-by: yolossn <sannagaraj@microsoft.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work @yolossn. Apart from some suggestions, everything looks great
columns={[ | ||
{ | ||
label: 'Class', | ||
getter: item => item.ingress.class, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same, please fix the object scope
columns={[ | ||
{ | ||
label: 'Class', | ||
getter: item => item.ingress.class, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same please edit scope
|
||
<IconButton | ||
onClick={() => { | ||
navigator.clipboard.writeText(item.spec.request); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same, please check for errors here
}; | ||
key: string; | ||
solver: { | ||
http01: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
according to cert-manager docs, should we also add for dns01
?
static kind = 'Order'; | ||
static apiName = 'orders'; | ||
static apiVersion = ['acme.cert-manager.io/v1']; | ||
static isNamespaced = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are namespaced resource
static isNamespaced = false; | |
static isNamespaced = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good stuff! left a couple of comments
{ | ||
id: 'approved', | ||
label: 'Approved', | ||
getValue: certificateRequest => certificateRequest.approved, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for simple cases like this, it's better to use datum
field like so:
{
id: 'approved',
label: 'Approved',
datum: 'approved',
}
import { NotInstalledBanner } from '../common/NotInstalledBanner'; | ||
|
||
export function CertificateRequestsList() { | ||
const [isManagerInstalled, setIsManagerInstalled] = useState(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this part (useState + useEffect) is repeated in each List
component. please extract it into a it's own hook.
this starts with value false which will render NotInstalledBanner by default, even if it is installed. I'd recommend adding additional field isManagerCheckLoading
that will keep track whether the check is still loading. this will avoid flashing "NotInstalledBanner"
also this check is loaded every time you enter the page, it should be fetched once and remember the value
}); | ||
|
||
// 1. certificates.cert-manager.io | ||
registerSidebarEntry({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this pattern repeats here, registerSidebarEntry
+ registerRoute
for Details and List could be put in a function
export class Certificate extends KubeObject<CertManagerCertificate> { | ||
static kind = 'Certificate'; | ||
static apiName = 'certificates'; | ||
static apiVersion = ['cert-manager.io/v1']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apiVersion can be just a string, no need for an array here
@@ -0,0 +1,12 @@ | |||
import { ApiProxy } from '@kinvolk/headlamp-plugin/lib'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this file could be named isCertManagerInstalled since there's nothing else here
@@ -0,0 +1,58 @@ | |||
import { KubeObject } from '@kinvolk/headlamp-plugin/lib/k8s/cluster'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/types/ folder name is a bit misleading. types usually means only typescript type definitions, here we define classes. Maybe /model/ would be better?
Testing the Cert-Manager Plugin
Prerequisites
Steps to Test
Clone the plugins repository:
Switch to the cert-manager branch:
Navigate to the cert-manager plugin directory:
cd cert-manager
Install the required dependencies:
Start the plugin in development mode:
Launch Headlamp. You should now see "Cert Manager" in the sidebar.
Optional: Generate Cert-Manager Types
To test the plugin with sample cert-manager resources:
Navigate to the test-files directory:
cd test-files
Apply the sample configurations to your cluster:
This will create:
Demo
cert-manager.mp4